🤖 Codex PR Review

🚫 Merge blocked: 1 serious issue(s) found in high-risk files

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in prompts/pr_review.md

The new reachability gate rule states: assign critical/high 'only when the supplied PR context proves … a current caller or entry point proven by the supplied PR context.' This is self-referential: it conditions the severity on what is present in the PR context supplied to the AI reviewer. A malicious or incomplete PR description that omits caller/entry-point information would cause the AI reviewer to systematically downgrade genuine critical vulnerabilities to medium/low rather than flagging missing evidence as itself suspicious. The rule as written incentivizes omission over disclosure, creating a systematic blind spot for security findings in PRs with sparse context. (line 23)

Suggestion: Add an explicit instruction: if the PR context lacks caller/entry-point evidence but the code change touches a known public boundary (e.g., HTTP handler, CLI entrypoint, deserialization path), the reviewer must flag the missing evidence as a medium finding and may still escalate to high/critical when the code path is self-evidently public. Distinguish between 'evidence is absent from PR description' and 'evidence proves non-reachability.'

ℹ️ Other Findings

1. 🟠 [HIGH] Logic in tests/test_run_codex_pr_review.py

The existing test test_review_prompt_requires_holistic_contract_review asserts that the prompt contains 'every identity-bearing integer' and 'one canonical timestamp representation' as unconditional requirements. However, the updated prompt (rule 8) now gates those JSON/wire contract checks behind the reachability rule 4, making them conditional. The test still passes because the strings are present in the prompt text, but the semantic contract the test is meant to enforce (that these checks are always required) is now broken—the test no longer validates the intended invariant. A future refactor that removes those strings from the conditional clause would still satisfy rule 8 while breaking the old guarantee, and this test would fail to catch it. (line 50)

Suggestion: Update test_review_prompt_requires_holistic_contract_review to assert that the JSON/wire contract checks appear in the conditional/gated form (i.e., the string 'Only for a public JSON/wire contract proven by' precedes those check descriptions), or rename/repurpose the test to reflect the new conditional semantics. Add a separate test asserting the conditionality is present.

2. 🟡 [MEDIUM] Logic in scripts/run_codex_pr_review.py

Rule 5 in the updated prompt prohibits requesting 'a new parser, store, registry, or event-persistence layer unless the changed code already exposes that current boundary.' This prohibition is absolute and applies even when the AI reviewer identifies a real security defect (e.g., unsanitized input fed into an existing parser) that genuinely requires a validation layer. The rule conflates 'do not invent new infrastructure for hypothetical risks' with 'never recommend any new protective layer,' which could suppress valid remediation suggestions for real findings. (line 334)

Suggestion: Narrow rule 5 to: 'Do not request a new parser, store, registry, or event-persistence layer solely to address a hypothetical future consumer or forged internal state.' Preserve the reviewer's ability to recommend protective layers (input validation, sanitization wrappers) when the defect is proven reachable under rule 4.

Review by Codex PR Review bot • PR

Goal Driver reachability arbitration

Current head: bd432fead44ed89842bc84519996cc00a5f31200

• The supplied PR context includes the unified diff, not only the PR description. A changed HTTP/CLI/deserialization entry point that is self-evidently public is therefore a current entry point proven by the supplied context and may still receive High/Critical severity. Sparse prose does not suppress code evidence.
• The existing holistic-contract test checks prompt content, while the new focused tests check that those wire checks are conditional on a proven public boundary. The reported test-name concern does not change runtime behavior or leave the new contract untested.
• Rule 5 already permits a protective layer when the changed code exposes the current boundary and the defect is reachable; it only rejects infrastructure requested for hypothetical risks.

Required actionlint, test, and Codex Review Gate checks are successful; current unresolved threads are zero. These repository-review findings are advisory under the merged authority split and do not justify a second closure.